Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

consolidate version information #32973

Merged
merged 4 commits into from
Jul 18, 2023
Merged

Conversation

radditude
Copy link
Member

@radditude radditude commented Apr 4, 2023

Now that we've switched to our new build system and are using version/VERSION as the canonical way to discover versions during the build process, it is unfortunately rather easy for version/VERSION and the version defined in version/version.go to drift out of sync, which causes unfortunate problems like #32963 at release time.

This PR proposes making version/VERSION the canonical source for version info at all times, using a file embed. To make things less confusing in local development, we'll keep the -dev prerelease by default and set the expectation that release builds, whether ours or a third-party build process, must specifically mark themselves as such via a linker flag in order to report the correct version.

Target Release

1.6.x

version/version.go Outdated Show resolved Hide resolved
version/version.go Outdated Show resolved Hide resolved
@radditude radditude force-pushed the radditude/one-version-to-rule-them-all branch from 891133f to a79795e Compare April 10, 2023 23:18
@radditude radditude force-pushed the radditude/one-version-to-rule-them-all branch from a79795e to 54b875a Compare April 10, 2023 23:45
@radditude radditude changed the title always read the version from the static file consolidate version information Apr 11, 2023
@radditude radditude force-pushed the radditude/one-version-to-rule-them-all branch 2 times, most recently from fb53e70 to d09db87 Compare April 11, 2023 00:12
@radditude radditude marked this pull request as ready for review April 11, 2023 00:51
@radditude radditude requested a review from a team April 11, 2023 00:51
@radditude radditude added the 1.4-backport If you add this label to a PR before merging, backport-assistant will open a new PR once merged label Apr 11, 2023
@radditude
Copy link
Member Author

I've thrown a backport label on here, but I'm very much open to the idea that we should not backport this build change to the 1.4 series, and instead let it be the default for 1.5 onwards.

@apparentlymart
Copy link
Contributor

FWIW the legacy release process includes some non-robust regular expressions looking for the Version and Prerelease variables in version.go and so it might no longer behave correctly after this change is merged.

I don't think it particularly matters since we have no intention to return to using the legacy process, but if we do end up needing to use the legacy process in any branch where this has been merged we should be sure to check that what it built has a correct version number compiled into it. (In practice I think the e2etests would catch it, because running terraform version to see if it returns the expected version number is one of the tests in that suite.)

@radditude
Copy link
Member Author

Excellent callout! I didn't preemptively open a PR over in the private repo since we haven't yet agreed that the path proposed in this PR is a good one, but I will certainly do that as followup.

Copy link
Contributor

@apparentlymart apparentlymart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of curiosity I triggered a run of the build workflow against this branch to see what it would do: https://github.com/hashicorp/terraform/actions/runs/4671153628

This all makes sense to me, so I'm cautiously approving it but with the caveat that I'm not an expert on all of the wiring that leads up to running the scripts you modified here and so although I think I can see how the information flows, it might be worth getting a second opinion from someone who was more involved in setting this up.


### Experimental Features

Experimental features of Terraform will be disabled unless `main.experimentsAllowed` is set to `yes`:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe worth calling out here that we only do this for alpha release builds, and recommend third-party distributors follow that same convention to reduce confusion.

BUILDING.md Outdated

## Go Options

The Terraform release process uses the Go toolchain defaults for the current operating system and processor architecture.
Copy link
Contributor

@apparentlymart apparentlymart Apr 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe we have some special rules for setting CGO_ENABLED differently on different platforms, which we should perhaps describe here in case the reader is trying to produce a binary that has identical behavior to the official release.

(In particular, if folks build a Linux binary on Linux without setting CGO_ENABLED=0 then they'll build an executable that relies on their system's specific libc, whereas our official releases have CGo disabled so that they can run on systems with any libc.)

@radditude radditude mentioned this pull request May 12, 2023
SemVer = semVerFull.Core()
Version = SemVer.String()

if dev == "no" {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there is already a prelease set in VERSION (e.g -alpha) would the desired behavior for local builds be that this is dropped in favour of -dev or the other way around?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dropping any existing prerelease in favor of -dev! That's been our convention previously, so seemed just as well to stick with it for now.

Instead of having two different places where we keep the current version
which must be manually kept in sync, let's use the same one that the
release process uses (version/VERSION).

Local builds will remain tagged with -dev by default, and we'll
disable this behavior with a linker flag at release time.
@radditude radditude force-pushed the radditude/one-version-to-rule-them-all branch from d09db87 to 1e2a6be Compare July 17, 2023 18:56
@radditude radditude removed the 1.4-backport If you add this label to a PR before merging, backport-assistant will open a new PR once merged label Jul 18, 2023
@radditude
Copy link
Member Author

I triggered a build from this branch and verified that the binaries produced by it report the correct version number: https://github.com/hashicorp/terraform/actions/runs/5581974639#artifacts

@radditude radditude merged commit 4530e36 into main Jul 18, 2023
41 checks passed
@radditude radditude deleted the radditude/one-version-to-rule-them-all branch July 18, 2023 17:42
@github-actions
Copy link

Reminder for the merging maintainer: if this is a user-visible change, please update the changelog on the appropriate release branch.

Copy link

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 13, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants